Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

11895: Fix buffer overrun in GetCryptoRandomString(). #7262

Merged
merged 2 commits into from
Dec 1, 2020
Merged

11895: Fix buffer overrun in GetCryptoRandomString(). #7262

merged 2 commits into from
Dec 1, 2020

Conversation

iefremov
Copy link
Contributor

@iefremov iefremov commented Nov 26, 2020 •
edited
Loading

Fixes brave/brave-browser#11895
Fixes brave/brave-browser#11897
Fixes brave/brave-browser#11898
Fixes brave/brave-browser#11899
Fixes brave/brave-browser#11900
Fixes brave/brave-browser#11901
Fixes brave/brave-browser#11902
Fixes brave/brave-browser#12923

Passing char* to Base64Encode was triggering an implicit
construction of StringPiece from a "string" which wasn't
null terminated. Using Base64Encode version that takes
a base::span will make us safe.

Resolves brave/brave-browser#11895

Submitter Checklist:

  • There is a ticket for my issue.
  • Used Github auto-closing keywords in the commit message.
  • Wrote a good PR/commit description
  • Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • Checked the PR locally: npm run test -- brave_browser_tests, npm run test -- brave_unit_tests, npm run lint, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed).
  • Requested a security/privacy review as needed.

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Adequate test coverage exists to prevent regressions
  • Major classes, functions and non-trivial code blocks are well-commented
  • Changes in component dependencies are properly reflected in gn
  • Code follows the style guide
  • Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

@iefremov
Copy link
Contributor Author

also cc @diracdeltas - most likely not a big deal, but looks like a security bug

@iefremov
Copy link
Contributor Author

Verified locally with asan that related tests are fixed

@iefremov iefremov mentioned this pull request Nov 27, 2020
Closed
23 tasks
@iefremov iefremov self-assigned this Nov 30, 2020
@iefremov
11895: Fix buffer overrun in GetCryptoRandomString().
02e2235 
Fixes brave/brave-browser#11895

Passing char* to Base64Encode was triggering an implicit
construction of StringPiece from a "string" which wasn't
null terminated. Using Base64Encode version that takes
a base::span will make us safe.
@iefremov
11895: Fix another memory bug and enable asan for problematic tests.
26f349b
@iefremov
Copy link
Contributor Author

iefremov commented Dec 1, 2020

CI failures are unrelated: known problem on mac and known test failure on linux

@iefremov iefremov merged commit b0b2632 into master Dec 1, 2020
@iefremov iefremov deleted the ie_random_span branch December 1, 2020 12:18
@iefremov iefremov added this to the 1.19.x - Nightly milestone Dec 1, 2020
@bsclifton
Copy link
Member

Nice work on this @iefremov! 😄 I set the milestone / assignee / labels for all the issues closed, should be good to go 👍

@iefremov
Copy link
Contributor Author

iefremov commented Dec 1, 2020

thanks @bsclifton !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ryanml ryanml ryanml approved these changes

@jumde jumde Awaiting requested review from jumde

@bsclifton bsclifton Awaiting requested review from bsclifton

Assignees

@iefremov iefremov

Labels
None yet
Projects
None yet
Milestone
1.19.x - Release
3 participants
@iefremov @bsclifton @ryanml